Add Tracer API #85
Add Tracer API #85
Conversation
danielkhan
requested review from
iredelmeier,
justindsmith,
mayurkale22 and
rochdev
as code owners
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 10 -2
Lines 200 152 -48
Branches 8 8
=====================================
- Hits 200 152 -48
|
| * | ||
| * @todo: Change return type once HttpTextFormat is available | ||
| */ | ||
| getHttpTextFormat: unknown; |
There was a problem hiding this comment.
This and getBinaryFormat can be replaced with inject and extract which are compatible with both as explained in #74 (comment).
The problem with having fixed methods by propagators is that it becomes impossible to add new formats which may be needed by vendors.
There was a problem hiding this comment.
There is no agreement on this as far as I can see and as long as it's in the spec I will leave it in here. I think you should post such concerns in the spec repo.
There was a problem hiding this comment.
This is clearly due to a limitation in Java. The spec specifies 2 formats which would be available using inject and extract.
There was a problem hiding this comment.
I would also prefer that because I think it would be nice to have the browser implementation of this not need code for the binary encoding if it doesn't use it (browser mostly speaks HTTP and can't do gRPC directly).
There was a problem hiding this comment.
Opened open-telemetry/opentelemetry-specification#174 in the meantime.
| * | ||
| * @todo: Move into module of its own | ||
| */ | ||
| export interface SpanOptions { |
There was a problem hiding this comment.
Could you please add an optional kind of type SpanKind, IsRecordingEvents boolean flag and parent span?
something like:
export interface SpanOptions {
kind?: SpanKind;
attributes?: Attributes;
isRecordingEvents?: boolean;
parent?: Span | SpanContext; (or childOf?: Span | SpanContext;)
startTime?: number;
}
WDYT?
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
This is not required for now, Could you please remove this file?
…metry-js into dkhan-add-tracer-api
|
I think I've addressed everything. Please revisit. |
| * @todo: Move into module of its own | ||
| */ | ||
| export interface SpanOptions { | ||
| kind?: SpanKind; |
There was a problem hiding this comment.
It would be nice to see block comment (/** … */) for each of these properties similar to other interfaces.
| /** | ||
| * | ||
| * @param name The name of the span | ||
| * @param options? SpanOptions used for span creation |
There was a problem hiding this comment.
MInor: I think we are using [] for optional param ex: [options].
|
Addressed minor edits. |
|
@rochdev please re-review |
| * @param [options] SpanOptions used for span creation | ||
| * @returns Span The newly created span | ||
| */ | ||
| start(name: string, options?: SpanOptions): Span; |
There was a problem hiding this comment.
As per the specs, the name should be startSpan instead of just start.
Adds the Tracer API.
This is still work in progress as some dependencies are not yet specified but it should provide a starting point and avoid blocking other work.